Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xgboost 0.90 (new formula) #43246

Closed
wants to merge 1 commit into from
Closed

xgboost 0.90 (new formula) #43246

wants to merge 1 commit into from

Conversation

ankane
Copy link
Contributor

@ankane ankane commented Aug 18, 2019

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@SMillerDev
Copy link
Member

Please don't specify the amount of jobs for make. That might cause issues on other systems.

@ankane
Copy link
Contributor Author

ankane commented Aug 18, 2019

My bad, updated

@javian javian added the new formula PR adds a new formula to Homebrew/homebrew-core label Aug 18, 2019
@javian
Copy link
Contributor

javian commented Aug 19, 2019

The docs say it requires gcc to enable multi-threading - do you think that would be necessary for this Formula?

@ankane
Copy link
Contributor Author

ankane commented Aug 19, 2019

Yeah, that'd be ideal. Just pushed an update.

@ankane
Copy link
Contributor Author

ankane commented Aug 19, 2019

It looks like brew linkage --test xgboost is failing for each build. However, I'm not sure how to reproduce it locally. Are you familiar with this error?

@ankane
Copy link
Contributor Author

ankane commented Aug 19, 2019

Sorry the noise. It was due to having gcc@8 marked as build when it's libgomp is needed during runtime. The formula now supports multi-threading.

:revision => "515f5f5c4779ff5361dcd796e22d55937e1048ea"

depends_on "cmake" => :build
depends_on "gcc@8"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to disagree with @javian there, but unless multithreading is essential to the app's behaviour, we generally avoid it. GCC is a heavy dependency, so we want to avoid it if at all possible, and compile with the system compiler.

Using libomp maybe help provide OpenMP support, if that's what you need.

And in any case, compiling with an older version of GCC is a definite “no way”.

Copy link
Contributor Author

@ankane ankane Sep 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @fxcoudert, thanks for the feedback on this as well. It sounds like @hcho3 has it working with libomp but it isn't merged yet, so will see what the maintainers want to do: dmlc/xgboost#4477

@ankane
Copy link
Contributor Author

ankane commented Sep 21, 2019

Hey @fxcoudert, I updated the formula to work with Apple Clang (disabled OpenMP). Also switched to make install (based on your feedback from the lightgbm formula). It looks like the build failed due to a credentials error.

@SMillerDev
Copy link
Member

@BrewTestBot test this please

@SMillerDev SMillerDev added the ready to merge PR can be merged once CI is green label Oct 7, 2019
@SMillerDev
Copy link
Member

Thanks @ankane! Without contributions like yours it'd be impossible to keep homebrew going with the high standards that users have come to expect from the project. You can feel good knowing that you've made the world a tiny bit better for homebrew users around the world! 👍 🎉

@fxcoudert fxcoudert closed this in 4d904cd Oct 7, 2019
@ankane
Copy link
Contributor Author

ankane commented Oct 7, 2019

Thanks @SMillerDev and @fxcoudert 👍

@lock lock bot added the outdated PR was locked due to age label Jan 7, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants